src: fix #9335, prevent duplicate txis in fluff queue. #20
Conversation
0xFFFC0000
force-pushed
the
dev/0xfffc/fluff_queue_duplicate
branch
from
b2333dd
to
a899386
Compare
There was a problem hiding this comment.
Thanks for working on this!
This would fix the issue but it includes another tx parse when adding txs to the fluff queue, which isn't ideal IMO.
Another way, because the txs are already sorted before being sent, is to just check sequential txs are not equal, if they are only add one of them to the message.
This does mean we still get duplicates in the queue but I don't think this isn't too big of a problem.
| { | ||
| cryptonote::transaction transaction; | ||
| if(cryptonote::parse_and_validate_tx_from_blob(tx, transaction)) | ||
| { | ||
| if(fluff_txs_hash.find(transaction.hash) != fluff_txs_hash.end()) | ||
| { | ||
| break; | ||
| } | ||
| fluff_txs_hash.emplace(transaction.hash); | ||
| } | ||
| fluff_txs.push_back(std::move(tx)); | ||
| break; | ||
| } |
There was a problem hiding this comment.
I don't think this is needed, right?
We already check for duplicates in the whole message at the top of this function.
There was a problem hiding this comment.
Aha. Saw the code here: https://github.com/0xFFFC0000/monero/blob/a899386881a79fecae1fbaf0fc463f33367599c6/src/cryptonote_protocol/cryptonote_protocol_handler.inl#L997
We don’t need the code here in cryptonote_protocol_handler.inl .
Thanks for mentioning it.
|
There is a minor issue. I was thinking about doing it without parse tx. But there was a malleability bug with varints where you could encode the value 0 as "\00", "\8000", "\808000", etc. So there may be two different blobs which have the same TXID. |
0xFFFC0000
force-pushed
the
dev/0xfffc/fluff_queue_duplicate
branch
2 times, most recently
from
efb82f0
to
79b7a45
Compare
Fix duplicate transaction monero-project#9335
0xFFFC0000
force-pushed
the
dev/0xfffc/fluff_queue_duplicate
branch
from
79b7a45
to
86264f3
Compare
|
Please retry analysis of this Pull-Request directly on SonarCloud |
Seed nodes for new stressnet
No description provided.